UCT/CUDA_COPY: Fix incorrect mem_flags detection#11635
Conversation
tomerg-nvidia
changed the title
tomerg-nvidia
force-pushed
the
fix_no_context_mem_flags
branch
from
f31ea76 to
65d1073
Compare
| mem_attr->alloc_length = addr_mem_info.alloc_length; | ||
| } | ||
|
|
||
| /* mem_flags detection requires a current context; push one if needed */ |
There was a problem hiding this comment.
maybe: DMABUF fd creation/export, also used to detect mem_flags, needs a cuda context
There was a problem hiding this comment.
removed it, now the push is much closer to the use
| CUcontext tmp_ctx; | ||
|
|
||
| UCT_CUDADRV_FUNC_LOG_WARN(cuCtxPopCurrent(&tmp_ctx)); |
There was a problem hiding this comment.
| CUcontext tmp_ctx; | |
| UCT_CUDADRV_FUNC_LOG_WARN(cuCtxPopCurrent(&tmp_ctx)); | |
| (void)UCT_CUDADRV_FUNC_LOG_WARN(cuCtxPopCurrent(NULL)); |
There was a problem hiding this comment.
Not relevant anymore
|
|
||
| UCT_CUDADRV_FUNC_LOG_WARN(cuCtxPopCurrent(&tmp_ctx)); | ||
| if (cuda_ctx == NULL) { | ||
| UCT_CUDADRV_FUNC_LOG_WARN(cuDevicePrimaryCtxRelease(cuda_device)); |
There was a problem hiding this comment.
| UCT_CUDADRV_FUNC_LOG_WARN(cuDevicePrimaryCtxRelease(cuda_device)); | |
| (void)UCT_CUDADRV_FUNC_LOG_WARN(cuDevicePrimaryCtxRelease(cuda_device)); |
More often than not, it is explicitly indicated that the return value is ignodred.
There was a problem hiding this comment.
not relevant anymore
| *cuda_ctx_p = NULL; | ||
| *cuda_device_p = CU_DEVICE_INVALID; |
There was a problem hiding this comment.
I think it's better to set values only in case of returning UCS_OK.
| ctx_pushed = (uct_cuda_copy_md_push_ctx(cuda_ctx, cuda_device) == | ||
| UCS_OK); |
There was a problem hiding this comment.
Shouldn't we return from the function if the method returns an error?
There was a problem hiding this comment.
Not relevant anymore. Also, for the dmabuf export the context push is best-effort to not change the current behavior which uses current context
| ctx_status = cuCtxGetCurrent(&cuda_ctx); | ||
| if ((ctx_status != CUDA_SUCCESS) || (cuda_ctx != nullptr)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Does the test really need to check this?
| UCS_TEST_P(test_mem_alloc_device, no_current_context_user_mem_registrable, | ||
| "CUDA_COPY_ASYNC_MEM_TYPE=cuda") | ||
| { | ||
| const size_t size = 4 * UCS_MBYTE; |
|
|
||
| mem_attr.field_mask = UCT_MD_MEM_ATTR_V2_FIELD_MEM_TYPE | | ||
| UCT_MD_MEM_ATTR_V2_FIELD_MEM_FLAGS; | ||
| query_status = uct_md_mem_query_v2(md(), (void*)dptr, size, &mem_attr); |
There was a problem hiding this comment.
static_cast can't do it because it's an integer to pointer conversion, used reinterpret_cast
And fix CR comments
tomerg-nvidia
force-pushed
the
fix_no_context_mem_flags
branch
from
f5d17e5 to
be6bdd2
Compare
tomerg-nvidia
changed the title
tomerg-nvidia
force-pushed
the
fix_no_context_mem_flags
branch
from
93c23d0 to
4f88c06
Compare
4 participants
What?
Push a cuda context before flag detection if the flag detection is going to export dmabuf fd.
If Memory is host-based vmm, skip the dmabuf fd check and just mark as registerable.
Why?
If there's currently no context and the memory flags detection is called the dmabuf export can fail, making it non-registerable.
Host-based vmm fail dmabuf fd export.
How?
Push context when needed, trying to use available context first